-
Notifications
You must be signed in to change notification settings - Fork 1
Feature/retrieve reserve identifiers #49
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Feature/retrieve reserve identifiers #49
Conversation
case journal; | ||
case lifecycleJournal; | ||
|
||
public function labels(): array |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not be a fixed list, but retrieved from the CODECHECK register (https://codecheck.org.uk/register/venues/index.json) or from the GitHub API (https://api.github.com/repos/codecheckers/register/labels).
The problem with the latter is, that there are labels that are irrelevant for the OJS plugin.
So, we probably need a configuration option that, for each journal, allows to select labels that are set to checks of that journal. Can you please create that one? There should be a selection of existing labels from GitHub.
|
||
enum CodecheckType | ||
{ | ||
case checkNL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might want to add different options here without a new release of the plugin, so this cannot be a fixed enum.
Note that you are mixing types and venues here, too.
The best way to get all types is probably to look at the property type
in register.json
: https://codecheck.org.uk/register/register.json
} | ||
} | ||
|
||
class CodecheckRegister extends Set |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not so happy with the name here, because this set is only about identifiers of the register. Please rename.
If you want to create a PHP representation of the whole register, then you should base it on register.csv
or register.json
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I maybe just call it something like CertificateList
or CertificateSet
because in the end it is just that.
My goal was not to create a PHP representation of the whole register, so something along the lines of names I just listed should work just fine.
'labels' => 'id assigned', // label | ||
'sort' => 'updated', | ||
'direction' => 'desc', | ||
'per_page' => 100, // get all issues in page |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's start with a lower number of issues, say... 20
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if there is by any chance no issue with the correct format inside these 20? The chance of that happening are very slim, but theoretically it would be possible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could just return an error and require editors to create the issue manually, but I do not like that.
I am a bit careful with how many request the plugin shall send to the GitHub API, but then the size of the response probably is not a reason for throttling.
Having a iterative approach here (retrieve 20, if no clear data then retrieve 20 more if need be, etc.) should work, and in most cases we can give a quick response in the UI then.
|
||
$this->client->authenticate($token, null, Client::AUTH_ACCESS_TOKEN); | ||
|
||
$repositoryOwner = 'dxL1nus'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use this repo for your testing: https://github.com/codecheckers/testing-dev-register
You can also create the existing labels etc. there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does it work with the GitHub Token there? Can I use my own one or do I need one especially provided to me to work for that repository?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use your own for your local development, you're not sharing that in the repository anyway.
For our demo server I can create one using a CODECHECK user, and other users of the plugin will probably have to add their own token.
$issueTitle = 'New CODECHECK | ' . $certificateIdentifier->toStr(); | ||
$issueBody = ''; | ||
$labels = ['id assigned']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this is information that will have to be provided by the plugin and the journal configuration, right?
Please think about a useful function/internal API or how a class could look like that provides all information that can be configured by a journal or provided from the submission (author name, unless anonymous; repo URL for the body; assigned codechecker, if already known; ...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would only work if we keep the reserve certificate
button at the very bottom of the form though correct? Because otherwise this information (like: repo URL
, assigned codechecker
) is most likely not yet known
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha, good point.
So let's go with "minimal information for reservation" first. Please create an issue about posting relevant information for the check in the issue. Probably we need something like an "Update information in register issue" workflow ?
Good first version! |
issue #11
CertificateRetrievingAndCreation.php
that fetches all open and closed Certificate identifiers with the right title format (title | certificate identifier
)identifier x/identifier y
) and gets all identifiers inside this rangeNew CODECHECK | new unique identifier
in my own repository (https://github.com/dxL1nus/dxL1nus/issues) to not disturb the ongoing CODECHECK register repository during our developmentThis last point would need to be changed in the future so that issues in the actual register are created